Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add InstrumentationLibrary for instrumentation library information #207

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp commented Sep 2, 2020

fix #204

As per spec, we should provide an optional param for version in TraceProvider::get_tracer. I think we should stick with spec and add second param here.

But it feels like most of the time people won't use it. So we maybe able to provide global::tracer and global::tracer_with_version methods so that users won't need to type None everytime when they retrive a tracer.
Not sure if it's desired, so I added second param into global::tracer for now.

…ntation library information.

* Fix problem that our instrumentation libraries only have name but don't have version
@TommyCpp TommyCpp requested a review from a team September 2, 2020 03:23
@jtescher
Copy link
Member

jtescher commented Sep 2, 2020

I think a second method may be more ergonomic, many people don't even like having to specify the first argument as brought up in open-telemetry/opentelemetry-specification#586.

@TommyCpp
Copy link
Contributor Author

TommyCpp commented Sep 2, 2020

I think a second method may be more ergonomic, many people don't even like having to specify the first argument as brought up in open-telemetry/opentelemetry-specification#586.

Make sense. Will add another method in global mod. But do we want to add a second method in TracerProvider as well?

@jtescher
Copy link
Member

jtescher commented Sep 2, 2020

Hm seems less critical as people are less likely to use TracerProvider directly anymore. Maybe better to go with single method there to encourage library developers to specify their versions 👍

If there is no version provided, we should probably just leave it to None instead of put a empty string there.
In most of the case, user wouldn't use version of instrumentation library. So it's best to by default allow user to get a tracer with only name.

Add another `trace_with_version` method so that if users want to, they can get a tracer with name and version information.
@TommyCpp TommyCpp force-pushed the instrumentation-library branch from 526198d to e575eda Compare September 2, 2020 21:28
Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jtescher jtescher merged commit ce469b4 into open-telemetry:master Sep 3, 2020
@TommyCpp TommyCpp deleted the instrumentation-library branch September 23, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add instrumentation library version information
2 participants